feat(storage): reclaim the prepared-request captures already on disk - #4738

Reviewed at exact head 82ff4a487d9aae0f7296d62c6c3efb9b36f18aa1. Five independent reviews ran on this, each sealing before reading the others. Four of them cleared the deletion itself. One found a reachable gap that I am raising as [P1], so I am not approving this head — everything else here is sound, and the fix looks small.

[P1] The retired-capture filter guards one of three selection paths

copyConversationArtifacts selects records in three passes. Only the first excludes the source, and its comment states the property the whole design depends on:

// A retired capture is on its way off disk and nothing reads one.
// Copying it would hand the new Session bytes already condemned,
// and would put records back after the sweep finished and stopped
// looking. Leaving them out also keeps the two from racing: the
// sweep can no longer delete a record this copy is holding.
record.source !== RETIRED_CAPTURE_ARTIFACT_SOURCE,

The next two passes do not look at source at all:

• linkedArtifacts (artifact-store.ts:440-451) matches on sessionId, id and status !== 'deleted', then pushes the record.
• includedArtifactIds (:453-463) matches on sessionId and the include/exclude sets.

This is reachable in production, not hypothetical. Side Conversation copy builds a linkedArtifacts request — kind === 'side_conversation' && linkedReferences.references.size > 0 — and one reviewer's probe confirmed by measurement that both unfiltered paths copy a capture into the target with source: provider_request_capture intact.

The consequences follow directly from the comment above:

1. Condemned bytes land in a new Session. The copy hands the target rows that are on their way off disk.
2. They are re-created after the sweep has finished. The sweep stops when nothing is left; a copy afterwards puts fresh provider_request_capture rows back, and the sweep is no longer looking.
3. A later sweep reclaims them, hollowing out the snapshot's rewritten artifactIds.
4. The race the comment says is prevented is not prevented on these paths. linkedArtifacts throws Linked Artifact <id> could not be copied when the record is gone — so a sweep that removes a capture between selection and copy turns a Side Conversation into a hard failure rather than a dropped key.

The PR summary's claim — "A conversation copy no longer carries retired captures over … which also stops it racing the sweep over the same record" — holds for the turn-scoped pass and not for the other two.

Smallest fix: apply the same source !== RETIRED_CAPTURE_ARTIFACT_SOURCE exclusion to all three selection passes. A regression should drive a real parent+child state through the Side Conversation coordinator, sweep, and then a real revision — asserting that ordinary child artifacts still copy.

What five reviews cleared

The deletion scope is correct. Exact-source deletion measured at 6/6 including tombstones and copy-rebuilt rows; all 14 remaining rows across the other 13 ArtifactSource values plus source-less rows survive; non-capture hardlink inodes and bytes stay readable.

Nothing else reads a capture reference. Enumerated independently by three seats, converging: the only production readers of captureArtifactId are the Core validator and conversation-copy.ts. Diagnostics and the ledger never dereference it.

Keeping captureArtifactId in the schema is load-bearing, not conservative. hasExactShape rejects any unknown key outright — measured directly: a record carrying the field validates while allowed contains it, and the same record fails the moment the key is removed from allowed. Dropping the field would strand exactly the records this change exists to free the bytes of.

The sweep's failure handling is sound. Five consecutive failures stop the loop in this process only — there is no persistent cursor, and the next Runtime Host start resumes over what remains. A successful batch resets the counter. stop() prevents the next batch rather than interrupting the current one, and an interrupted batch leaves a purge intent that recovery replays.

Two defensive observations that do not block, from the cross-team seat:

• Closure does not wait for an in-flight sweep. stop() sets a flag and close proceeds to storage.close; if closure lands between file deletion and the metadata write, the batch can delete its targets and fail the metadata write, leaving a purge intent that the next recovery replays. The target set is still limited to retired captures. Worth having stop await the in-flight batch, or folding it into a unified drain.
• On a large store each batch still resolves removal entries across every non-batch record as a path guard, so the 16–256 adaptive batch cannot shorten lock hold time — shrinking the batch does not help. That is live-turn latency rather than wrong deletion.

(The reference-filter finding above was reached independently by two seats and, from the opposite direction, by a third that had classified it as unreachable in production until the Side Conversation caller was identified.)

Everything except the filter gap is in good shape, and the premise — that these bytes are duplicates nothing reads — is well supported.

// 一条退役 capture 正在离开磁盘,而且没有人读它。
// 复制它等于把已被判定要删的字节交给新 Session,
// 并且会在 sweep 结束、不再查看之后把记录放回去。
// 把它们排除在外也让两者不再竞争:
// sweep 不能再删掉这次复制正持有的记录。
record.source !== RETIRED_CAPTURE_ARTIFACT_SOURCE,

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Correction to my previous comment: the finding above is real, but I graded it P1 on a production-reachability claim I had not established. It should be [P2]. Same head, 82ff4a487d9aae0f7296d62c6c3efb9b36f18aa1.

What still holds

The filter inconsistency is real and reads directly from the code. copyConversationArtifacts selects in three passes; only the turn-scoped one excludes RETIRED_CAPTURE_ARTIFACT_SOURCE. linkedArtifacts (artifact-store.ts:440-451) and includedArtifactIds (:453-463) do not look at source.

Feeding a capture id to the explicit include path does copy it, with source intact — measured. So the defensive gap is not theoretical: any caller that reaches these passes with a capture id re-creates a condemned row, and a later sweep reclaims it.

What I got wrong

I wrote that Side Conversation makes this reachable in production, on the basis that its copy builds a linkedArtifacts request. That the request is built is true. That a capture id can travel in it is what I did not verify, and it does not follow.

linkedArtifacts is populated from collectConversationCopyLinkedChildReferences, which walks tool results and collects only typed linked-child references — content.kind === 'subagent' (content.artifactIds) and content.kind === 'agent_swarm'. Those are child-run outputs a tool declared. A provider_request_capture is written by the runtime per provider request; it is not a tool result artifact, so nothing puts it in that set. The graph reference validator then checks session, status and turn lineage — no source check, but also no path I can show that admits a capture in the first place.

The same applies to the include path in production: it is fed from collectConversationCopySessionFileRefs, which filters to session_file refs. Reproducing the include copy required standing up an advanced owner client and disguising a capture as a typed session_file — a constructed precondition, not a production route.

So the honest statement is: the three passes disagree about source, and that is worth fixing before some future caller does reach them — but I cannot show a current production path that does. That is a defensive gap, P2, which is where the first reviewer to enumerate this had it before I raised it. I moved it up too fast, on a code-shaped inference rather than a demonstrated route.

This does not change the fix, which is still to apply the exclusion to all three passes — cheap, and it closes the gap regardless of whether anything reaches it today. It does change the urgency, and it means this head has no P0 or P1 from my side. Everything I listed as cleared in the previous comment stands unchanged, including the measured deletion scope, the single pair of captureArtifactId readers, and the load-bearing schema field.

If someone can demonstrate a real subagent or agent_swarm tool result whose declared artifactIds include a capture — historical data would be the place to look — that would restore the reachability argument and the higher grade with it.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Retracting my previous correction. The original [P1] stands, and I have verified the reachability chain myself this time. I also think the fix on 17b36a21 is incomplete in a way that matters — details below.

Where my correction was wrong

I wrote that linkedArtifacts is populated only from typed linked-child references, so a provider_request_capture — written by the runtime, not produced by a tool — could never travel in one. The first half is true. The second does not follow, and I asserted it without checking what actually fills those artifactIds.

A child result's artifactIds is not a list of tool outputs. It is every artifact on the child's turn:

• listTurnArtifacts filters on turnId and status !== 'deleted' only — no source filter (artifact-store.ts:729-731).
• listArtifactsForTurn passes it straight through (execution-composition.ts:1065-1066).
• Three production paths write the whole list into the typed result: artifactIds: artifacts.map((artifact) => artifact.id) at session-manager.ts:2923, :3260 and :3349 — none filters by source.

And a child's captures are exactly artifacts on that turn. A Host end-to-end test deleted by #4722 shows the consequence directly: after a real child run, listTurnArtifacts returns exactly one record, asserted to be source: 'provider_request_capture', and the parent's decoded subagent result is asserted to carry that same id list. A second case asserts a child's capture count equals its provider-request count and that the typed result carries all of them.

From there the rest already held: conversation-copy.ts:772-803 projects those ids into linked-child references, and session-revision-graph-references.ts:244-255 resolves each with getInSession, failing operation_unavailable if one is gone.

So this is not a future caller. Ledgers written by any Session that ever spawned a subagent or swarm already contain capture ids inside typed child results. I had the shape of the argument and stopped one step short of the data.

The fix closes the copy, but the historical ids are still live

17b36a21 applies isRetiredCapture to all three selection passes, and the extraction is the right shape — the predicate's comment names the real invariant, that the property belongs to the copy rather than to one of its passes. The tests cover ordinary linked artifacts still copying, the include path skipping a capture, and the linked path rejecting one.

But that third behaviour meets the historical data head-on. linkedArtifacts now excludes captures from its find, so the lookup misses and throws Linked Artifact <id> could not be copied. Given a typed subagent result that already lists a capture id, this means:

• before the sweep, the copy now throws where it previously succeeded;
• after the sweep, getInSession in the graph-reference validator fails first with operation_unavailable.

Either way a Side Conversation or revision over a Session with a historical subagent result fails rather than degrades — and the throw is newly introduced by this fix, on data that already exists on disk.

Rejecting a caller that names a capture outright is defensible. Rejecting a caller that named it because the runtime put it there is a different case, and it is the common one. Worth considering: drop condemned ids from a linked-child request rather than failing it — the capture carries nothing a copy needs, which is the same reasoning that lets capturedArtifactJoin drop the key instead of throwing.

One seat is currently verifying exactly this — whether a swept Session with a historical capture id can still take a revision or Side Conversation on this head. I will report that result either way; if it passes, this concern is void and only the grading correction stands.

Grading

Back to [P1], on reachability that is now demonstrated rather than inferred: production writes capture ids into typed child results today, and both the pre-fix copy and the post-fix rejection are reachable from ordinary data. Everything else in my first comment — the measured deletion scope, the single pair of captureArtifactId readers, the load-bearing schema field, the sweep's failure handling — is unchanged and still clear.

Apologies for the churn. The second correction is on me: I inferred a boundary from the shape of the code instead of following the data that crosses it, which is the same mistake in the opposite direction.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Re-reviewed at 17b36a21ff2edd8db59abefa582a6cf4692663a2. The three-pass filter is correctly closed, but [P1] is not — the fix moves the failure earlier rather than removing it. This is the verification I said I would report either way, and it came back negative.

Both timings fail, measured

Rebuilt Storage on this head and ran a probe through the real SQLite store, the production typed-result collector, and the production graph validator, with a typed agent_swarm result whose artifactIds is [capture, tool-result] — the shape production writes today:

The last row isolates the cause: the capture id is the only thing standing between these Sessions and a working copy.

Before this fix, a Side Conversation over that data copied. On this head it throws before the sweep, and the unrecoverable failure after the sweep is unchanged. The new rejection is introduced by the fix, on data already on disk.

The new test locks in a premise the history disproves

artifact-store.test.ts:395-405 makes "naming a capture in linkedArtifacts must fail" a contract. That is right for a caller that names one deliberately — but it holds only if a capture cannot end up in a typed child result, and it can.

SessionManager.finalizeAndListChildTurnArtifacts() still applies no source filter, and all three completion/recovery paths still write the whole list (session-manager.ts:2923, :3260, :3349). The Host end-to-end tests removed by #4722 show the outcome on real runs: a local-read child's turn has exactly one artifact, asserted to be a capture, and the parent's typed subagent result is asserted to carry that same id list; the implementation-child case asserts every provider request's capture reaches the typed result alongside the tool-result and writeback. So the caller is not naming a capture — the runtime put it there.

What the fix needs

Two parts, because filtering at copy selection is too late:

1. Project confirmed retired-capture ids out of the linked-child request before validation treats them as required references. By the time copyConversationArtifacts selects, the graph validator has already resolved each id.
2. Preserve or rewrite retired-source identity before deletion — after the sweep the source metadata is gone, so nothing downstream can tell "a capture that was reclaimed" from "a tool-result that is genuinely missing". Alternatively, hold back captures still named by a typed result. Degrading every unknown missing artifact is not an option — that is the distinction the current design depends on everywhere else.

Confirmed closed, and one observation to keep

The copy-selection gap itself is properly fixed: isRetiredCapture (artifact-stores.ts:103-106) is applied at all three passes and at purge. Ordinary linked artifacts still copy, which the new tests assert. The adaptive batch became a fixed 256, and the comment now states plainly why a smaller batch cannot shorten a live turn's wait — the fixed cost is per batch, so shrinking it only makes the residue pay the same toll more times. That is a good response to the earlier scheduling note.

stop() still does not wait for the in-flight batch (artifact-stores.ts:301-303 returns a flag setter; the loop reads it only after the current purge and pause). Unchanged from the previous head and still worth folding into a unified drain.

Verification on this head: Storage build; Artifact store 50/50; Runtime Host graph refs 14/14; the new probe reproduces both failures; merge tree against current main clean; hosted test and windows_recovery both green. The green CI is accurate — nothing existing tests cover is broken. The failure needs a Session with a historical subagent or swarm result, which no test constructs.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Approving at exact head 9b73fa9d81d604ec6db1c174a929d46a8700ad94. The P1 is closed — both failures I reported are gone, measured on the same probe that found them.

Verified closed

Same real SQLite store, same production typed agent_swarm result:

Focused 90/90, three packages build, both exact-head checks green, merge tree clean.

The fix is better than what was asked for

I suggested projecting condemned ids out of the linked-child request. What landed is the more general and more correct statement: a child result names every Artifact its turn held, in a ledger that can never be rewritten, so an id in it outlives whatever it named. What the validator checks is therefore whether a reference reaches outside its own child and lineage — not whether its target survived.

That reframing dissolves the problem I could not see a way around. I had thought the fix would need to preserve retired-source identity before deletion, because after the sweep nothing downstream can distinguish "a capture that was reclaimed" from "a tool-result that is genuinely missing". Under this rule the distinction is not needed: neither one is the reader's problem, and both resolve to nothing. linkedArtifacts moving from if (!record) throw to if (record) selected.push(...) follows from the same sentence.

Agent Graph revision references outlive the Artifacts they name covers it, and the existing reject invalid ownership boundaries is untouched.

One comment to narrow

The new comment says a reference "that crosses a Session or a lineage was never admissible and still fails". The lineage half holds; the Session half does not, and cannot.

getInSession(childSessionId, artifactId) resolves against that Session's own snapshot, so a foreign-Session id is not found and returns record: null — indistinguishable from a missing one. It therefore takes the new continue, and the record.sessionId !== childSessionId branch below is unreachable for that case.

Harmless in practice: the downstream copy resolves against the child Session too, so foreign data cannot be copied in, and an ordinary typed result never produces such an id. Not worth grading. But the comment currently claims a guard that the lookup shape removes, and a future reader may rely on it. Narrowing it to lineage would make it accurate.

One observation carried forward

stop() still does not wait for the in-flight batch — it sets a flag (artifact-stores.ts:301-303) and the loop reads it only after the current purge and pause, while close proceeds to storage.close. Unchanged across all three heads. An interrupted batch leaves a purge intent that recovery replays, so this is not a correctness hole; it is still worth folding into a unified drain.

Everything cleared in the earlier rounds stands: the measured deletion scope, captureArtifactId's two production readers, the load-bearing schema field, FIFO ordering and purge-intent recovery, and five-failure stop being per-process rather than permanent. The adaptive batch became a fixed 256 with a comment that states plainly why a smaller batch cannot shorten a live turn's wait.

This is a feature, so the merge decision remains a human's.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.